Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs(batch): explain record type discrepancy in failure and success handler #2868

Merged
merged 5 commits into from
Jul 28, 2023
Merged

docs(batch): explain record type discrepancy in failure and success handler #2868

merged 5 commits into from
Jul 28, 2023

Conversation

duc00
Copy link
Contributor

@duc00 duc00 commented Jul 28, 2023

Issue number: #2816

Summary

This PR adds details to the Batch Processor / Extending BatchProcessor section of the docs.

Changes

  • misc rewording for more clarity
  • add warning about type discrepancy of record passed arg to success_handler and failure_handler
  • complete example with a success_handler override case

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change? No

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@duc00 duc00 requested a review from a team as a code owner July 28, 2023 16:09
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Jul 28, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 28, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 28, 2023

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

heitorlessa
heitorlessa suggested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome recommendations 🚀 Loved the attention to detail in improving the code example and its name.

Requested minor changes and we can merge right afterwards.

docs/utilities/batch.md Outdated Show resolved Hide resolved
docs/utilities/batch.md Outdated Show resolved Hide resolved
docs/utilities/batch.md Outdated Show resolved Hide resolved
docs/utilities/batch.md Outdated Show resolved Hide resolved
@heitorlessa heitorlessa changed the title docs(batchprocessor): mention success/failure handlers record arg discrepancy docs(batch): explain record type discrepancy in failure and success handler Jul 28, 2023
@duc00
Copy link
Contributor Author

duc00 commented Jul 28, 2023

@heitorlessa, thanks. Changes addressed. I also removed the reference to [Accessing processed messages] since every needed explanation is now in the warning.

heitorlessa
heitorlessa suggested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last brush up after I visualized locally; I'm accepting them and merging before I head for the weekend ;)

docs/utilities/batch.md Outdated Show resolved Hide resolved
docs/utilities/batch.md Outdated Show resolved Hide resolved
@heitorlessa heitorlessa added the batch Batch processing utility label Jul 28, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heitorlessa
Copy link
Contributor

Merging. I've changed warning to note to make it less triggering for some audience, and remove the Example call out so we only have one where they'd pay attention to.

image

@heitorlessa heitorlessa merged commit 920d70e into aws-powertools:develop Jul 28, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 28, 2023

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@heitorlessa heitorlessa linked an issue Jul 28, 2023 that may be closed by this pull request
Feature request: Fix inconsistency between BatchProcessor success_handler and failure_handler call arguments #2816
Closed
2 tasks
@duc00
Copy link
Contributor Author

duc00 commented Jul 28, 2023

Great, thanks @heitorlessa. Enjoy the weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heitorlessa heitorlessa heitorlessa approved these changes

Assignees
No one assigned
Labels
batch Batch processing utility documentation Improvements or additions to documentation size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: Fix inconsistency between BatchProcessor success_handler and failure_handler call arguments
2 participants
@duc00 @heitorlessa